Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement converters in sigmatch #384

Closed
wants to merge 14 commits into from
Closed

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Jan 18, 2025

refs #381

Implemented with a converter section in the index like (converter (kv <type> <converter>)), was just my first guess for what to do, maybe there is a better way. The converter symbol is stored rather than the offset which I'm hoping is fine, I tried offsets first and couldn't figure out what to do with them.

Attaches to nominal return types for now skipping ref/ptr like type bound ops, both because the algorithm already exists and just storing symbol IDs in the index is simpler. Unlike type bound ops, attachment is required to use converters, so this means only converters to nominal types work. An idea for structural types would be to store the root magic but still skip magics like ref/ptr, so array[I, T] and ref array[3, int] both attach to array but T or ref T cannot be attached to anything. For converters that can't be attached to anything we could also attach them to . and always consider these.

Only generics are left but looks like a can of worms

@Araq
Copy link
Member

Araq commented Jan 19, 2025

Maybe it's better not to do that in sigmatch but in sem.nim instead. Outline:

const
  ConverterSecretName = "`converter`"

proc converterKey(c: var SemContext; n: TypeCursor): string =
  var n = n
  while n.kind in {MutT, SinkT, PtrT, RefT, OutT, InvokeT}:
    inc n
  result = ConverterSecretName & toString(n, false)

proc converterMatch(c: var SemContext; m: var Match; f, a: TypeCursor;
                    apos: int; info: PackedLineInfo): bool =
  # we speed this up by checking if type `a` can have any converter attached to it:
  let k = converterKey(c, a)
  var converters: seq[SymId] = @[]

  let localConverters = c.m.iface.getOrDefault(k)
  for defId in items localConverters:
    converters.add defId

  for imported in items c.importedModules:
    let candidates = c.p[imported].iface.getOrDefault(k)
    for defId in candidates:
      converters.add defId

  var orig = NoSymId
  result = false
  for conv in items converters:
    # f(a)
    # --> f(conv(a)) ?
    # conv's return type must match `f`.
    # conv's input type must match `a`.
    let retType = returnType(conv)
    let inputType = firstParam(conv)

    # first match the input argument of `conv` so that the unification algorithm works as expected:
    let res = typeRel(inputType, a)
    var resB = Different
    case res
    of Same, Subtype:
      resB = typeRel(f, retType)
    of Generic:
      let retTypeInst = instReturnType(retType, context.bindings)
      resB = typeRel(f, retTypeInst)
    of Different:
      discard
    if resB != Different:
      if orig == NoSymId:
        orig = conv

        m.hiddenCalls[apos] = r
        inc m.usedConverters

        m.argTypes[apos] = f

        result = true
      else:
        # ambiguous match. For now don't report:
        result = false
        break

To be called from resolveOverloads if idx < 0.

@metagn
Copy link
Collaborator Author

metagn commented Jan 19, 2025

Going to see if I can rearrange it

@metagn metagn changed the title implement converters implement converters in sigmatch Jan 21, 2025
@metagn metagn mentioned this pull request Jan 21, 2025
3 tasks
@metagn
Copy link
Collaborator Author

metagn commented Jan 21, 2025

There is an off chance this will come up again but #399 is fairly ahead to the point that it's probably better to redo it after rather than continue this, so closing

@metagn metagn closed this Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants